Improve the performance of CachedIdentifiersExtractor by dunglas · Pull Request #1257 · api-platform/core

dunglas · 2017-07-19T17:49:17Z

Q	A
Bug fix?	no
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	n/a
License	MIT
Doc PR	api-platform/docs#1234

Follows #1256.

soyuka · 2017-07-19T18:10:48Z

hmm tests failed though

dunglas · 2017-07-19T18:13:11Z

I'm on it

meyerbaptiste · 2017-07-19T18:32:47Z

Could we not do the same for all Cached* classes?

teohhanhui · 2017-07-19T18:35:11Z

    private $cacheItemPool;
    private $propertyAccessor;
    private $decorated;
+    private $memoryCache = [];


Naming... See #1256 (comment)

dunglas · 2017-07-19T20:42:48Z

I've tried something but I would like a review @soyuka. I'm not sure I respect your initial intent.

soyuka · 2017-07-20T07:19:12Z

-        $cacheIsHit = false;
+        $keys = $this->getKeys($item, function ($item) use (&$identifiers) {
+            return $identifiers = $this->decorated->getIdentifiersFromItem($item);
+        });


this is cool stuff!

🤷‍♂️

Using a reference to an unknown variable and declaring it in a closure looks cool to me haha 😅

soyuka · 2017-07-20T07:50:45Z

@meyerbaptiste this one is slightly different and more complex then metadata classes. Initial PR #997

meyerbaptiste · 2017-07-20T09:01:28Z

Yep @soyuka, I posted my message before he refactored his PR. My previous comment is now more relevant for #1256.

soyuka · 2017-07-20T09:07:04Z

Yeah I agree we have a lot of repetition in those "cached" classes, finally it's always the same:

check cache (key is PREFIX + md5(resourceClass))
if cached return cached value
if no cache call decorator and cache

Do you have an idea to remove the duplicated code (only diff is the injected decorator and the cache key prefix)?

teohhanhui · 2017-07-20T09:11:35Z

Is it safe for us to use Symfony\Component\Cache\Adapter\ArrayAdapter with $storeSerialized = false? Since our metadata classes are all immutable... And Symfony\Component\PropertyInfo\Type is immutable too.

teohhanhui · 2017-07-20T09:13:43Z

In the case of CachedIdentifiersExtractor we are storing an array of strings, so it's "immutable" too.

dunglas · 2017-07-20T09:23:28Z

ArrayAdapter will be slower (more function calls) and - as pointed out by Nicolas Grekas on Twitter - is not part of cache.system because it's always slower than a local cache. Symfony components use a memory cache too.

dunglas · 2017-07-20T09:25:34Z

We can probably go one step further for this class: it should be possible to cache (in a memory cache) the results as well.

teohhanhui · 2017-07-20T09:29:25Z

-        $this->assertEquals(['id' => 1], $identifiersExtractor->getIdentifiersFromItem($dummy));
+        $expectedResult = ['id' => 1];
+        $this->assertEquals($expectedResult, $identifiersExtractor->getIdentifiersFromItem($dummy));
+        $this->assertEquals($expectedResult, $identifiersExtractor->getIdentifiersFromItem($dummy), 'Trigger the local cache');


How does this test verify that we are triggering the local cache? Also, it should not matter in a unit test.

He's hacking coverage to please @Simperfit 😆

Actually, this second call is crucial to test correct behaviour, i.e. that we return the correct value when cache is used. But it's not testing whether we're triggering the local cache.

Oh wait, we already have different test cases for that... Lol

Not really for the coverage. You're right @teohhanhui, it doesn't verify that the local cache actually works (it's very hard to test reliably), however it tests if the local cache doesn't introduce a bug.

For instance, if some one contribute this patch:

-return $this->memoryCache[$resourceClass]; +return $this->memoryCache[$resourceC];

The test will detect the problem. Without this line, it will not.

@dunglas Doesn't testSecondPass already do that?

Good catch. testSecondPass should be update to bypass the local cache IMO (by creating a new instance).

$localCache is unambiguous. #preach

soyuka · 2017-07-20T09:30:33Z

@dunglas I don't think that caching values is worth it. Also cache invalidation will be an issue. This is already improving performances a lot because we avoid calling the whole metadataFactory thing from IdentifierExtractor to find identifiers (simple and sufficient IMO).

…tifiers Improve the performance of CachedIdentifiersExtractor

soyuka approved these changes Jul 19, 2017

View reviewed changes

teohhanhui reviewed Jul 19, 2017

View reviewed changes

dunglas force-pushed the improve_cached_identifiers branch from 649d81d to 4d4b5c4 Compare July 19, 2017 20:42

api-platform deleted a comment from bco-trey Jul 19, 2017

Improve the performance of CachedIdentifiersExtractor

b68cbbf

dunglas force-pushed the improve_cached_identifiers branch from 4d4b5c4 to b68cbbf Compare July 19, 2017 21:21

soyuka approved these changes Jul 20, 2017

View reviewed changes

soyuka reviewed Jul 20, 2017

View reviewed changes

Simperfit approved these changes Jul 20, 2017

View reviewed changes

dunglas merged commit c6a7bef into api-platform:master Jul 20, 2017

dunglas deleted the improve_cached_identifiers branch July 20, 2017 07:50

teohhanhui reviewed Jul 20, 2017

View reviewed changes

teohhanhui mentioned this pull request Jul 20, 2017

Rename memoryCache to localCache in CachedIdentifiersExtractor #1263

Merged

hoangnd25 pushed a commit to hoangnd25/core that referenced this pull request Feb 23, 2018

Merge pull request api-platform#1257 from dunglas/improve_cached_iden…

9e4148b

…tifiers Improve the performance of CachedIdentifiersExtractor

soyuka mentioned this pull request Jun 3, 2026

"@OA\\Items() parent type must be "array" in " #6194

Closed

Uh oh!

Uh oh!

Conversation

dunglas commented Jul 19, 2017

Uh oh!

soyuka commented Jul 19, 2017

Uh oh!

dunglas commented Jul 19, 2017

Uh oh!

meyerbaptiste commented Jul 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dunglas commented Jul 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soyuka commented Jul 20, 2017

Uh oh!

meyerbaptiste commented Jul 20, 2017

Uh oh!

soyuka commented Jul 20, 2017

Uh oh!

teohhanhui commented Jul 20, 2017

Uh oh!

teohhanhui commented Jul 20, 2017

Uh oh!

dunglas commented Jul 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dunglas commented Jul 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dunglas Jul 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soyuka commented Jul 20, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dunglas commented Jul 20, 2017 •

edited

Loading

dunglas Jul 20, 2017 •

edited

Loading